Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shehab wchar fix #11

Merged
merged 16 commits into from
Jul 10, 2024
Merged

Shehab wchar fix #11

merged 16 commits into from
Jul 10, 2024

Conversation

A-Shehab
Copy link
Collaborator

@A-Shehab A-Shehab commented May 8, 2024

According to ISO/IEC 9899/1999 section 7.19.2: Streams

wide character input/output functions when applied to a stream should change the file orientation to be wide-oriented

before this change wide character input/output functions were applied to
byte-oriented stream resulting in erroneous results handling wide characters
input/output to/from files.

@A-Shehab A-Shehab requested a review from keith-packard as a code owner May 8, 2024 08:21
@A-Shehab A-Shehab force-pushed the shehab_wchar_fix branch from d574633 to 5a594ff Compare May 8, 2024 09:26
@A-Shehab A-Shehab force-pushed the shehab_wchar_fix branch from 5a594ff to 277abec Compare May 29, 2024 12:03
@@ -540,6 +540,7 @@ foreach target : targets
'test-ungetc-ftell',
'test-fgetc',
'test-fgets-eof',
'test-wchar',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to run this test on the native library to make sure picolibc and glibc (the usual native library) agree on these semantics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test('test-wchar',
         executable('test-wchar',
                    'test-wchar.c',
                    c_args: native_c_args,
                    link_args: native_c_args,
                    dependencies: native_lib_m))

@keith-packard, I'm not 100% sure about my understanding of the meson.build file, do you mean i need to add the test like that at line 625 instead of what i did ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but you need to add the native testing in addition to the regular test -- that way when doing "native" testing, we'll test the picolibc implementation and then also test the host C library implementation to validate the test itself. So, add what you've got above somewhere around line 625, yes, but also add 'test-wchar', to the list of simple tests.

This file is just a wrapper for picolibc.h. Delete unnecessary
references to newlib.h or picolibc.h and replace the remaining ones
with sys/features.h as that defines all of the features the library
supports, not just those defined by the build system.

Signed-off-by: Keith Packard <[email protected]>
Make sure any .c file includes picolibc.h before testing build
parameters.

Signed-off-by: Keith Packard <[email protected]>
Make this match other asm source in the library, even
though it doesn't strictly need to as it doesn't use
any C preprocessor stuff.

Signed-off-by: Keith Packard <[email protected]>
Remove C library-specific bits for glibc or picolibc as the
code is just plain-old POSIX and doesn't need any magic.

Signed-off-by: Keith Packard <[email protected]>
Make sure library feature test macros are defined before
being used.

Signed-off-by: Keith Packard <[email protected]>
This file includes the regular strftime implementation, which also
defines _DEFAULT_SOURCE. Fix that by undefining before including.

Signed-off-by: Keith Packard <[email protected]>
Tinystdio uses a bunch of feature macros before including any headers.

Signed-off-by: Keith Packard <[email protected]>
Go through all of the machine-specific source code and make sure
picolibc.h is included so that all of the feature test macros are set
before being used.

Signed-off-by: Keith Packard <[email protected]>
These come from glibc where various 64-bit explicit APIs are provided
under the protection of _LARGEFILE64_SOURCE, which is defined by
_GNU_SOURCE.

Replace the __LARGE64_FILES stuff with these and then use _GNU_SOURCE
where needed as that exposes these APIs. We could use
_LARGEFILE64_SOURCE instead, but it seems better to just expose the
GNU API instead.

Signed-off-by: Keith Packard <[email protected]>
This sets up a sample interrupt vector to show how this works.

Signed-off-by: Keith Packard <[email protected]>
This mirrors the existing __signbit and __signbitf functions and
allows a complete implementation of the non-builtin signbit macro.

Change the internal inline version name to signbitl_inline like
the other similar functions.

Signed-off-by: Keith Packard <[email protected]>
Make sure all functions are declared fully in header files before
being defined or used.

Signed-off-by: Keith Packard <[email protected]>
This was needed before all source files were fixed to ensure
all feature test macros were defined before being used by
including sys/features.h

Signed-off-by: Keith Packard <[email protected]>
Exposes these when _POSIX_C_SOURCE >= 199309L:

int	getc_unlocked (FILE *);
int	getchar_unlocked (void);
void	flockfile (FILE *);
int	ftrylockfile (FILE *);
void	funlockfile (FILE *);
int	putc_unlocked (int, FILE *);
int	putchar_unlocked (int);

The I/O functions don't do anything different as tinystdio doesn't
have any locking at the top level APIs. Getting these to poke down
inside the buffered layer would improve performance at a pretty
significant cost in complexity.

flockfile/funlockfile is implemented using the global libc lock. This
should at least serialize threads which all use this API. ftrylockfile
is the same as flockfile as there's no try-lock API to use.

Signed-off-by: Keith Packard <[email protected]>
According to ISO/IEC 9899/1999 section 7.19.2: Streams

before this change wide character input/output functions were applied to
byte-oriented stream resulting in erroneous results handling wide characters
input/output to/from files.

Signed-off-by: Ahmed Shehab <[email protected]>
Added testcase that checks input/output wide-chars to/from a file.
issue found by running SuperTest by Solid Sands

Signed-off-by: Ahmed Shehab <[email protected]>
@mostafa-salmaan mostafa-salmaan merged commit b26c565 into main Jul 10, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants